clamp boundary.circle.radius for reverse queries #1618
Conversation
| @@ -1,10 +1,9 @@ | |||
| var geo_common = require ('./_geo_common'); | |||
| var _ = require('lodash'); | |||
| var defaults = require('../query/reverse_defaults'); | |||
| var LAT_LON_IS_REQUIRED = true, | ||
| CIRCLE_IS_REQUIRED = false; | ||
|
|
||
| const non_coarse_layers = ['venue', 'address', 'street']; |
There was a problem hiding this comment.
Huh, I wonder when we last used it... We have long needed to improve reverse geocoding support for custom layers, as mentioned in #1161. Not something to solve in this PR though.
| const raw = { | ||
| 'point.lat': '12.121212', | ||
| 'point.lon': '21.212121', | ||
| 'boundary.circle.radius': '1km' // note the 'km' |
There was a problem hiding this comment.
this is weird, I'd like to change it, but in another PR.
right now you can specify 5km or 5m and in both cases the unit designation is stripped and results in 5, which is km.
There was a problem hiding this comment.
Ah yeah that is unfortunate. Very Javascript-y 😆
missinglink
force-pushed
the
clamp-boundary-circle-reverse
branch
from
b0ddb04 to
44ea887
Compare
test/unit/sanitizer/_geo_reverse.js
Outdated
| t.equals(raw['boundary.circle.lat'], 12.121212); | ||
| t.equals(raw['boundary.circle.lon'], 21.212121); | ||
| t.equals(raw['boundary.circle.radius'], '0.000000000000001'); |
There was a problem hiding this comment.
I think you can skip testing the raw values, since you specified them above.
IMO it just makes the test longer and it took me a second to realize it was checking the raw value, not clean. At first glance it looked to me like the functionality is not working and the radius is not being clamped!
There was a problem hiding this comment.
yeah good point, I just copied the existing format, it also caught me out writing the tests.
I guess the purpose is to show that the $raw values are not mutated, I can have a single test for that and clean up th other ones.
There was a problem hiding this comment.
Hmm actually no, that's not right, because if so, then the raw values would all be strings 🤷♂️
Anyway, I just cleaned up the tests by removing the assertions for $raw
| const raw = { | ||
| 'point.lat': '12.121212', | ||
| 'point.lon': '21.212121', | ||
| 'boundary.circle.radius': '1km' // note the 'km' |
There was a problem hiding this comment.
Ah yeah that is unfortunate. Very Javascript-y 😆
| var LAT_LON_IS_REQUIRED = true, | ||
| CIRCLE_IS_REQUIRED = false; | ||
|
|
||
| const non_coarse_layers = ['venue', 'address', 'street']; |
There was a problem hiding this comment.
Huh, I wonder when we last used it... We have long needed to improve reverse geocoding support for custom layers, as mentioned in #1161. Not something to solve in this PR though.
missinglink
force-pushed
the
clamp-boundary-circle-reverse
branch
from
44ea887 to
9f741b1
Compare
I noticed that the
boundary.circle.radiusparam is currently unbounded for/v1/reverse.If a users sets this value very high it can result in a lot of disk I/O as ES pages in the
docvaluesto score all the results.This PR defines minimum and maximum values for this radius value, I've selected
5kmas the max and an arbitrarily low value of0.00001kmfor the min; although I'm open to changing either of these.